Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: extends the input type of zonesNormalize function #267

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jobo322
Copy link
Member

@jobo322 jobo322 commented Nov 5, 2024

there is a package that use zonesNormalize but those zones has the structure:

{ id: string } & FromTo

so should be nice to refactor this function to accept those kind of inputs and preserve in this particular case the id

@jobo322
Copy link
Member Author

jobo322 commented Nov 5, 2024

should we have zones if we pass only exclusion zones? and the input zones is empty?

Copy link
Member

@lpatiny lpatiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@targos This is not a time critical code and I think it make sense to have this here.
WDYT ?

@targos
Copy link
Member

targos commented Nov 11, 2024

What I care about is consistency. Please also do it in the other functions of the zones folder.

@targos
Copy link
Member

targos commented Nov 11, 2024

Also, this seems to be a breaking change (see failing tests)

@jobo322
Copy link
Member Author

jobo322 commented Nov 12, 2024

Also, this seems to be a breaking change (see failing tests)

This method in particular can returns zones with an empty zones input. e.a

const result = zonesNormalize([], {
      from: 0,
      to: 10,
      exclusions: [{ from: 1, to: 2 }],
    });

this breaking change can be avoided by:

if (zones.length === 0) {
    zones.push({ from, to } as Zone);
  }

@jobo322 jobo322 force-pushed the refactor-zonesNormalize-05-11-2024-433 branch from 43c84e9 to 289c587 Compare November 12, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants